feat(Popper): add dir prop for RTL/LTR support#2610
Conversation
📝 WalkthroughWalkthroughAdds optional ChangesPopper direction support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/Popper/PopperContent.vue (1)
177-182: Newdirprop wiring looks correct.
dir?: Directionis optional and correctly typed against the sharedDirectionalias. Pairing withuseDirection(computed(() => props.dir))at Line 233 properly falls back toConfigProvider’s direction and finally'ltr', preserving existing behavior for unset props.One small note: the phrasing “when applicable” in the JSDoc is a bit vague — consider clarifying that the direction only affects the computed
transform-origin(specifically thestart/endalignment on top/bottom placements when the arrow is hidden), and does not automatically flipside. Users in RTL layouts who wantside: 'right'to visually appear on the logical start still need to mirrorsidethemselves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Popper/PopperContent.vue` around lines 177 - 182, Update the JSDoc for the dir prop in PopperContent.vue to explicitly state that dir?: Direction (wired to useDirection(computed(() => props.dir))) only affects computed transform-origin (i.e., start/end alignment on top/bottom placements when the arrow is hidden) and does not flip or remap the side prop — users must mirror side themselves in RTL; make the wording concise and replace "when applicable" with this specific behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/Popper/PopperContent.vue`:
- Around line 177-182: Update the JSDoc for the dir prop in PopperContent.vue to
explicitly state that dir?: Direction (wired to useDirection(computed(() =>
props.dir))) only affects computed transform-origin (i.e., start/end alignment
on top/bottom placements when the arrow is hidden) and does not flip or remap
the side prop — users must mirror side themselves in RTL; make the wording
concise and replace "when applicable" with this specific behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6de00e3e-048d-4dbb-96dd-7ebbccc66361
📒 Files selected for processing (2)
packages/core/src/Popper/PopperContent.vuepackages/core/src/Popper/utils.ts
zernonia
left a comment
There was a problem hiding this comment.
Thanks for working on RTL support for Popper 🙏 The X/Y split in transformOrigin is the right idea, but I think the feature is incomplete as-is and I'd like to request a change before merging.
The dir prop currently only influences the animation origin, not the actual placement. Floating UI decides RTL alignment-flipping from the computed direction of the floating wrapper element (isRTL → getComputedStyle(floating).direction, where floating is our floatingRef wrapper). But this PR sets :dir only on the inner <Primitive> and feeds the prop solely into transformOrigin.
So the two can disagree — e.g. <PopperContent dir="rtl" /> inside an LTR document positions with LTR alignment (wrapper inherits ltr) but animates from the RTL origin, landing on one side while scaling from the opposite corner.
Could you also bind :dir="dir" on the floatingRef wrapper so Floating UI's isRTL and transformOrigin stay consistent and the prop actually drives placement?
<div
ref="floatingRef"
data-reka-popper-content-wrapper=""
:dir="dir"
...
>Minor follow-ups:
- Reuse the shared
Directiontype inutils.tsinstead of inlining'ltr' | 'rtl'. - A test covering the RTL flip (and placement/origin consistency) would be great, since the snapshot is the only thing exercising it right now.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/Popper/utils.ts (1)
14-18: ⚡ Quick winConsider documenting the default direction behavior.
The
dirparameter is optional, and when omitted the code defaults to LTR behavior (sinceoptions.dir === 'rtl'evaluates tofalsewhendirisundefined). This implicit default is reasonable but not self-documenting.📝 Suggested improvement
Consider either:
- Adding a JSDoc comment documenting that
dirdefaults to LTR when omitted, or- Making the default explicit:
export function transformOrigin(options: { arrowWidth: number arrowHeight: number dir?: Direction }): Middleware { + const dir = options.dir ?? 'ltr' return { name: 'transformOrigin', options, fn(data) { const { placement, rects, middlewareData } = data const cannotCenterArrow = middlewareData.arrow?.centerOffset !== 0 const isArrowHidden = cannotCenterArrow const arrowWidth = isArrowHidden ? 0 : options.arrowWidth const arrowHeight = isArrowHidden ? 0 : options.arrowHeight const [placedSide, placedAlign] = getSideAndAlignFromPlacement(placement) const noArrowAlignX = { - start: options.dir === 'rtl' ? '100%' : '0%', + start: dir === 'rtl' ? '100%' : '0%', center: '50%', - end: options.dir === 'rtl' ? '0%' : '100%', + end: dir === 'rtl' ? '0%' : '100%', }[placedAlign]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/Popper/utils.ts` around lines 14 - 18, The transformOrigin middleware accepts an optional dir on the options object but silently treats undefined as LTR; update transformOrigin to make the default explicit and/or document it: either (a) add a JSDoc above the transformOrigin function stating "dir defaults to 'ltr' when omitted" and mention how it's used, or (b) normalize the value inside transformOrigin by assigning a default (e.g., const dir = options.dir ?? 'ltr') before any checks (references: transformOrigin and options.dir) so the LTR default is obvious and maintained.packages/core/src/Popper/Popper.test.ts (1)
68-110: 💤 Low valueConsider adding a test for default
dirbehavior.The current tests verify that explicitly passing
dir='rtl'ordir='ltr'correctly sets the wrapper attribute. Consider adding a test case for whendiris omitted to document the expected default behavior.🧪 Example test for default behavior
it('uses default dir when not explicitly provided', async () => { const DefaultPopper = defineComponent({ setup() { return () => h(PopperRoot, null, { default: () => [ h(PopperAnchor, null, { default: () => 'Anchor' }), h(PopperContent, {}, { default: () => 'Content' }), ], }) }, }) const wrapper = mount(DefaultPopper, { attachTo: document.body }) const wrapperDiv = wrapper.find('[data-reka-popper-content-wrapper]') // Document expected default - likely 'ltr' based on useDirection behavior expect(wrapperDiv.attributes('dir')).toBeDefined() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/Popper/Popper.test.ts` around lines 68 - 110, Add a test to verify the default dir behavior when PopperContent is rendered without a dir prop: create a component similar to RtlPopper/LtrPopper that renders PopperRoot with PopperAnchor and PopperContent (no dir prop), mount it, find the element with data-reka-popper-content-wrapper, and assert the wrapper's dir attribute is set (or equals the expected default like 'ltr'); reference the existing test helpers and components PopperRoot, PopperAnchor, PopperContent and the selector [data-reka-popper-content-wrapper] to locate the element.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/Popper/Popper.test.ts`:
- Around line 28-66: Add unit tests to expand direction-aware coverage for
transformOrigin: reuse baseOptions, makeData and call
transformOrigin(...).fn(...) to assert that top-start/top-end flip X like bottom
(expect RTL start => '100%', LTR start => '0%'); add left/right placement tests
ensuring Y alignment is direction-neutral (e.g., makeData('left-start') and
assert rtl.data.y === ltr.data.y === '0%'); add a test where dir is undefined to
assert default LTR behavior (e.g., transformOrigin({ ...baseOptions
}).fn(makeData('bottom-start')) yields x === '0%'); and add an arrow-visible
case by passing middlewareData.arrow with width/height to makeData and asserting
dir does not change arrow-based origin results.
---
Nitpick comments:
In `@packages/core/src/Popper/Popper.test.ts`:
- Around line 68-110: Add a test to verify the default dir behavior when
PopperContent is rendered without a dir prop: create a component similar to
RtlPopper/LtrPopper that renders PopperRoot with PopperAnchor and PopperContent
(no dir prop), mount it, find the element with data-reka-popper-content-wrapper,
and assert the wrapper's dir attribute is set (or equals the expected default
like 'ltr'); reference the existing test helpers and components PopperRoot,
PopperAnchor, PopperContent and the selector [data-reka-popper-content-wrapper]
to locate the element.
In `@packages/core/src/Popper/utils.ts`:
- Around line 14-18: The transformOrigin middleware accepts an optional dir on
the options object but silently treats undefined as LTR; update transformOrigin
to make the default explicit and/or document it: either (a) add a JSDoc above
the transformOrigin function stating "dir defaults to 'ltr' when omitted" and
mention how it's used, or (b) normalize the value inside transformOrigin by
assigning a default (e.g., const dir = options.dir ?? 'ltr') before any checks
(references: transformOrigin and options.dir) so the LTR default is obvious and
maintained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ab24d0c-95df-4112-8cab-b228cb30a7ee
⛔ Files ignored due to path filters (1)
packages/core/src/Popper/__snapshots__/Popper.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
packages/core/src/Popper/Popper.test.tspackages/core/src/Popper/PopperContent.vuepackages/core/src/Popper/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/Popper/PopperContent.vue
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/Popper/Popper.test.ts (1)
108-130: ⚡ Quick winConsider adding edge case: arrow data with
centerOffset !== 0.The current tests cover two scenarios:
- No arrow data (
arrowDataundefined) →isArrowHidden = true- Arrow data with
centerOffset: 0→isArrowHidden = falseThe upstream
transformOriginlogic has a third path: arrow data exists butcenterOffset !== 0, which setsisArrowHidden = true. While this likely behaves the same as scenario 1, testing it explicitly would verify that the no-arrow alignment (direction-awarenoArrowAlignX) is correctly applied when the arrow cannot be centered.🧪 Example test case
it('centerOffset !== 0 triggers no-arrow alignment (direction-aware)', async () => { const withArrow = { arrowWidth: 10, arrowHeight: 5 } const cannotCenter = { x: 20, y: 15, centerOffset: 5 } // non-zero offset // Should use noArrowAlignX, not arrow center expect((await run({ ...withArrow, dir: 'rtl' }, 'bottom-start', cannotCenter)).x).toBe('100%') expect((await run({ ...withArrow, dir: 'ltr' }, 'bottom-start', cannotCenter)).x).toBe('0%') })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/Popper/Popper.test.ts` around lines 108 - 130, Add a test that covers the arrow-data-but-not-centerable branch: create cannotCenter = { x: 20, y: 15, centerOffset: 5 } and reuse withArrow = { arrowWidth: 10, arrowHeight: 5 }, call run({ ...withArrow, dir: 'rtl' }, 'bottom-start', cannotCenter) and run(..., 'bottom-start', cannotCenter) with dir: 'ltr', and assert the returned .x uses the no-arrow alignment values ('100%' for rtl, '0%' for ltr) to ensure the transformOrigin/arrow-centering logic (the run helper and transformOrigin path that checks centerOffset and isArrowHidden) behaves as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/Popper/Popper.test.ts`:
- Around line 108-130: Add a test that covers the arrow-data-but-not-centerable
branch: create cannotCenter = { x: 20, y: 15, centerOffset: 5 } and reuse
withArrow = { arrowWidth: 10, arrowHeight: 5 }, call run({ ...withArrow, dir:
'rtl' }, 'bottom-start', cannotCenter) and run(..., 'bottom-start',
cannotCenter) with dir: 'ltr', and assert the returned .x uses the no-arrow
alignment values ('100%' for rtl, '0%' for ltr) to ensure the
transformOrigin/arrow-centering logic (the run helper and transformOrigin path
that checks centerOffset and isArrowHidden) behaves as expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d486f80-ee26-4b73-a209-78d49b4e411a
📒 Files selected for processing (1)
packages/core/src/Popper/Popper.test.ts
|
@zernonia |
🔗 Linked issue
❓ Type of change
📚 Description
📸 Screenshots (if appropriate)
📝 Checklist
Summary by CodeRabbit
New Features
Tests